Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove jandex plugin from flaky test #15153

Merged
merged 1 commit into from
Feb 19, 2021
Merged

Remove jandex plugin from flaky test #15153

merged 1 commit into from
Feb 19, 2021

Conversation

glefloch
Copy link
Member

This removes the jandex plugin from a sample test project. On windows, it looks the jandex task was running after the jar task which is not correct.

@quarkus-bot quarkus-bot bot added the area/gradle Gradle label Feb 17, 2021
@glefloch glefloch requested a review from gsmet February 17, 2021 21:41
@geoand
Copy link
Contributor

geoand commented Feb 18, 2021

Thanks @glefloch.

It seems like the Gradle tests are failing on Windows, is it related?

@glefloch
Copy link
Member Author

Yes, this test fails on multiple PRs and I try to fix it. But it looks like my fix isn't enough. I will keep working on it.

@glefloch glefloch force-pushed the fix/gradle-included-build-test branch from 0b474cc to ff3b979 Compare February 18, 2021 19:08
@quarkus-bot quarkus-bot bot added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Feb 18, 2021
@glefloch
Copy link
Member Author

@aloubyansky I updated the include build discovery process and removed the usage of the GradleConnector.
By looking for the QuarkusModel all substitution needed to have the quarkus plugin enabled which should not be the case as we can substitute any library.
As Gradle always output resources in build/resources/main and classes in build/classes/java/main or build/classes/kotlin/main, we can easily build those paths from the project path.
Also on windows, there was a weird behavior regarding task ordering, so this should also fix it. WDYT?

gastaldi
gastaldi reviewed Feb 18, 2021
View reviewed changes
devtools/gradle/src/main/java/io/quarkus/gradle/builder/QuarkusModelBuilder.java Outdated
private void addSubstitutedProject(final DependencyImpl dep, File projectFile) {
dep.addPath(new File(projectFile, MAIN_RESOURCES_OUTPUT));
File classesOutput = new File(projectFile, CLASSES_OUTPUT);
for (File languageDirectory : classesOutput.listFiles()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listFiles may return null if classesOutput isn't a valid directory

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I will add a check on this one.

aloubyansky
aloubyansky reviewed Feb 18, 2021
View reviewed changes
devtools/gradle/src/main/java/io/quarkus/gradle/builder/QuarkusModelBuilder.java Outdated
for (File sourceDirectory : moduleOutput.getSourceDirectories()) {
dep.addPath(sourceDirectory);
private void addSubstitutedProject(final DependencyImpl dep, File projectFile) {
dep.addPath(new File(projectFile, MAIN_RESOURCES_OUTPUT));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this path always exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a check too, we have seen some project with no resource directory.

@aloubyansky
Copy link
Member

That is supposed to cover the default layout, right? It could still be different in some projects?

@glefloch
Copy link
Member Author

@aloubyansky, the output layout should not differ a lot. Usually, there is custom layout for tests.
So yes, if projects use something else that src/main/java, src/main/kotlin or src/main/scala for sources, then it won't be handled but I think most project respect src/main for project sources.
If we encounter another layout, we could load a built-in gradle model to get more infos about the project and handled that.

@glefloch glefloch force-pushed the fix/gradle-included-build-test branch from ff3b979 to 68539a7 Compare February 18, 2021 20:19
@aloubyansky
Copy link
Member

Makes sense, thanks!

@glefloch glefloch force-pushed the fix/gradle-included-build-test branch from 68539a7 to 567c953 Compare February 18, 2021 20:47
@gsmet
Copy link
Member

gsmet commented Feb 19, 2021

CI looks good (at least on the Gradle front) but I'm not really qualified to review this PR.

@aloubyansky if everything is OK for you, I let you approve and merge?

@aloubyansky aloubyansky merged commit 6dce423 into quarkusio:master Feb 19, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi left review comments

@aloubyansky aloubyansky aloubyansky approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle
Projects
None yet
Milestone
1.13.0.CR1
Development

Successfully merging this pull request may close these issues.
5 participants
@glefloch @geoand @aloubyansky @gsmet @gastaldi